Skip to content

MemoryWriteStream: enforce single-flush semantics - #93

Merged
Tyrrrz merged 6 commits into
primefrom
copilot/memory-write-stream-flush-once
Jul 24, 2026
Merged

MemoryWriteStream: enforce single-flush semantics#93
Tyrrrz merged 6 commits into
primefrom
copilot/memory-write-stream-flush-once

Conversation

Copilot AI commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

MemoryWriteStream.Flush() could be called multiple times with no error, silently re-copying the buffer to the underlying stream on each call. It should only be flushed once.

Changes

  • MemoryWriteStream: Added _flushed flag; subsequent manual Flush() calls throw InvalidOperationException. Added Dispose(bool) override that sets a _disposing guard before calling base.Dispose() (which internally calls Flush()), so disposal after a prior manual flush is silently ignored. The guard is reset after disposal so that erroneous Flush() calls post-dispose still throw.
  • MemoryWriteStreamTests: Two new tests covering the double-flush-throws and flush-then-dispose-ignores behaviors.
using var stream = new MemoryWriteStream(destination);
stream.Write([1, 2, 3]);
stream.Flush();          // OK
stream.Flush();          // throws InvalidOperationException
stream.Dispose();        // OK — silently ignored

@Tyrrrz Tyrrrz left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread PowerKit/MemoryWriteStream.cs Outdated
Comment thread PowerKit/MemoryWriteStream.cs Outdated
Copilot AI requested a review from Tyrrrz July 24, 2026 11:14
@Tyrrrz
Tyrrrz marked this pull request as ready for review July 24, 2026 11:18
Copilot AI review requested due to automatic review settings July 24, 2026 11:18
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.51%. Comparing base (3c1e3f1) to head (3d9153f).

Additional details and impacted files
@@            Coverage Diff             @@
##            prime      #93      +/-   ##
==========================================
+ Coverage   86.36%   86.51%   +0.15%     
==========================================
  Files          78       78              
  Lines        1393     1409      +16     
  Branches      239      242       +3     
==========================================
+ Hits         1203     1219      +16     
  Misses        137      137              
  Partials       53       53              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to enforce single-flush semantics in MemoryWriteStream, preventing repeated Flush() calls from silently re-copying the in-memory buffer into the underlying destination stream.

Changes:

  • Track flush state in MemoryWriteStream and throw on subsequent manual Flush() calls.
  • Update disposal behavior to avoid throwing when Dispose() triggers flush after a prior manual flush.
  • Add tests for double-flush throwing and flush-then-dispose not throwing.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
PowerKit/MemoryWriteStream.cs Adds flushed-state tracking and routes flush/dispose through a guarded path.
PowerKit.Tests/MemoryWriteStreamTests.cs Adds tests for double-flush error behavior and flush-then-dispose behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread PowerKit/MemoryWriteStream.cs
Comment thread PowerKit.Tests/MemoryWriteStreamTests.cs
Copilot AI requested a review from Tyrrrz July 24, 2026 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants